-
Notifications
You must be signed in to change notification settings - Fork 793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hidden members should not be shown in QuickInfo #73
Conversation
Great! Looks good to me, though see one comment below. Especially good to see the Language Service unit test added. |
@@ -1505,6 +1505,7 @@ module private TastDefinitionPrinting = | |||
not (isInterfaceTy denv.g oty) | |||
| [] -> true) | |||
|> List.filter (fun v -> denv.showObsoleteMembers || not (HasFSharpAttribute denv.g denv.g.attrib_SystemObsolete v.Attribs)) | |||
|> List.filter (fun v -> not (Infos.AttributeChecking.CheckFSharpAttributesForHidden denv.g v.Attribs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NicePrint.fs code is used to show the signature of things when emitting signature files using the "-sig:file" option. So I think you need to show obsolete, hidden and experimental members in that case, because the signature file should be "logically complete".
For this, you should add a flag "showHiddenMembers" to DisplayEnv with default value "false", and set this to "true" in BuildInitialDisplayEnvForDocGeneration.
I actually think "showObsoleteMembers" should match this and be "false" by default, but set to "true" in BuildInitialDisplayEnvForDocGeneration. I don't think we want obsolete members shown in class/module tooltips.
Finally, rename BuildInitialDisplayEnvForDocGeneration to BuildInitialDisplayEnvForSigFileGeneration.
@latkin @KevinRansom - I went looking for an "under review" label to label the pull request with - I kind of like it if all issues and pull requetss get a label of some kind - if you agree could you create a label for this? thanks! |
I added a test for the obsolete case and added the changes you suggested. But now we have a new issue: the test is still red. Any ideas? |
I took a look and didn't spot any problem with the code. Do both tests fail or just the "obsolete" one? Is v.Attribs empty for both Print1 and Print2? |
only the obsolete test fails. Both properties (Print1 and Print2) have 0 attribs. If I add the HiddenAttribute to Print1 in the obsolete test, I get 1 attribute (Obsolete attr is still missing) and Print1 is no longer displayed. So somehow it doesn't find the ObsoleteAttribute, but the HiddenAttribute works. |
You may have to open "System" or use "System.Obsolete" |
scratch that. I cleaned my repo, added |
OK, great, then this looks good to go. |
just for my understanding: the attribute wasn't found since the code didn't compile. Is that correct? |
this fixes #50
I added a testcase and I used
CheckFSharpAttributesForUnseen
to ask for thehidden
attribute.I had to split
CheckFSharpAttributesForUnseen
sinceNicePrint.fs
has a modedenv.showObsoleteMembers
which allows to show obsolete members butCheckFSharpAttributesForUnseen
checked for hidden and obsolote.